Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: refactor to use os.stat and os.lstat instead of unsafe C calls #20759

Merged
merged 6 commits into from
Feb 8, 2024

Conversation

syrmel
Copy link
Contributor

@syrmel syrmel commented Feb 8, 2024

Goal: simplify os codebase and reduce number of unsafe blocks.

Changes:

  • Introduce os.lstat() to call C.lstat explicitly to be consistent with the behavior of C.stat() and C.lstat()
  • Refactor the following functions to use os.stat() or os.lstat(): file_size, cp, is_executable, is_dir, is_link, kind_of_existing_path, file_last_mod_unix, posix_set_permission_bit, and inode
  • Add comments to the Stat struct

When appropriate because of Win API instead of C lib calls, split functions between os_stat_default.c.v or os_stat_windows.c.v.

Regression tested with existing os tests.

fn eprintln_unknown_file_size() {
eprintln('os.file_size() Cannot determine file-size: ' + posix_get_error_msg(C.errno))
}

// file_size returns the size of the file located in `path`.
// If an error occurs it returns 0.
// Note that use of this on symbolic links on Windows returns always 0.
pub fn file_size(path string) u64 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check, if that works with files larger than 4GB on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed; works on 32- and 64-bit Windows

Copy link
Member

@spytheman spytheman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent cleanup.

@spytheman spytheman merged commit 410bd9d into vlang:master Feb 8, 2024
48 checks passed
@syrmel syrmel deleted the os-stat branch February 8, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants